Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename ParentOrElse sampler to ParentBased and enhance it according to the spec #1153

Merged
merged 6 commits into from
Sep 10, 2020

Conversation

matej-g
Copy link
Contributor

@matej-g matej-g commented Sep 9, 2020

Relevant specification change from open-telemetry/opentelemetry-specification#610

More specifically, the changes include:

  • Renaming of ParentOrElse sampler to ParentBased
  • fallback sampler renamed to root
  • Allowing to set samplers for all applicable cases, when span has a parent (remoteParentSampled, remoteParentNotSampled, localParentSampled, localParentNotSampled); if not set, appropriate defaults are used in accordance with the spec
  • Updated sampler description
  • Adjusted tests and added new tests to cover changes

Resolves #1084

Matej Gera added 2 commits September 9, 2020 21:16
- Renaming of type and sampler function
- Enhancing ParentBased with sampler options
- Set default samplers for each applicable parent-based case
- Adjust ShouldSample(...) func accordingly
- add tests for ParentBased sampler options and description
- renaming in trace_test.go
@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #1153 into master will increase coverage by 0.2%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1153     +/-   ##
========================================
+ Coverage    77.7%   77.9%   +0.2%     
========================================
  Files         135     135             
  Lines        7153    7191     +38     
========================================
+ Hits         5559    5603     +44     
+ Misses       1350    1344      -6     
  Partials      244     244             
Impacted Files Coverage Δ
sdk/trace/provider.go 91.7% <100.0%> (ø)
sdk/trace/sampling.go 100.0% <100.0%> (+16.2%) ⬆️
api/trace/api.go 65.5% <0.0%> (ø)
api/trace/tracetest/provider.go 100.0% <0.0%> (ø)
api/trace/tracetest/mock_tracer.go 65.0% <0.0%> (ø)
bridge/opentracing/internal/mock.go 67.8% <0.0%> (ø)

sdk/trace/sampling.go Outdated Show resolved Hide resolved
sdk/trace/sampling.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the correct approach. Only issue is the configuration format.

sdk/trace/sampling.go Outdated Show resolved Hide resolved
- More clearer naming of structs; add comments where missing
- Adhere to the configuration style guide
@matej-g
Copy link
Contributor Author

matej-g commented Sep 10, 2020

Thanks for the feedback, I reworked the configuration so that it adheres to the styling guide, hopefully now all is A-OK.

sdk/trace/sampling.go Outdated Show resolved Hide resolved
sdk/trace/sampling.go Outdated Show resolved Hide resolved
sdk/trace/sampling.go Outdated Show resolved Hide resolved
sdk/trace/sampling.go Outdated Show resolved Hide resolved
sdk/trace/sampling.go Outdated Show resolved Hide resolved
sdk/trace/sampling.go Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit 0041e2d into open-telemetry:master Sep 10, 2020
KasonBraley added a commit to KasonBraley/opentelemetry-go-contrib that referenced this pull request Feb 16, 2023
Update the examples to use the current API for configuring production sampling.
These names were changed a couple of years ago.

open-telemetry/opentelemetry-go#1115
open-telemetry/opentelemetry-go#1153
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename ParentOrElse to ParentBased and generalize to support all cases
4 participants